Bug fix for OAuth m2m scopes#178
Merged
Merged
Conversation
Updated m2m authenticator to use "all-apis" scope. Added a new constructor function for m2m authenticator that allows client to pass in additional scopes. Signed-off-by: Raymond Cypher <raymond.cypher@databricks.com>
rcypher-databricks
requested review from
andrefurlan-db,
jadewang-db,
susodapop and
yunbodeng-db
as code owners
November 17, 2023 18:10
andrefurlan-db
approved these changes
Nov 17, 2023
jackyhu-db
approved these changes
Nov 17, 2023
mani-mathur-arch
added a commit
that referenced
this pull request
Jul 19, 2026
Point KERNEL_REV at f299577, the current head of kernel PR #178 (configurable HTTP retry / backoff policy). The prior pin (ffa73c0) is a direct ancestor — f299577 just adds the retry→HttpConfig merge test and cloudfetch chunk clamp on top. All C-ABI symbols this backend calls, including set_retry_config and set_proxy, are present in the header at the new rev. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
added a commit
that referenced
this pull request
Jul 21, 2026
The previous pin (f299577) was the PR head of the kernel's retry/backoff change (#178). That PR has now squash-merged to kernel main as 9ac3f3d, so the PR-head ref is no longer a stable target — the kernel-lib source build (git fetch + checkout KERNEL_REV) would break once that ref is cleaned up. Re-pin to the merged main SHA 9ac3f3d, which carries the same C-ABI surface (kernel_session_config_set_retry_config + the cloudfetch_max_chunks_in_memory session-conf key). No driver code change: the committed header is gitignored and regenerated from the pinned checkout at build time. Verified: kernel-lib rebuilt at 9ac3f3d, tagged build (-tags databricks_kernel) links clean, kernel + arrowscan unit tests green. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
added a commit
that referenced
this pull request
Jul 21, 2026
…nn/error telemetry, token-caching docs, configurable backoff (#412) The genuinely-new SEA-via-kernel driver work from the new-items backlog: the short PuPr telemetry/config/Geography tail plus advanced proxy and configurable backoff, followed by a datatype-parity + decimal-perf + RSS pass. Each is re-implemented clean on `main` (post-#399) — the pre-#399 POC was the spec, not the source. Every commit was reviewed with Isaac Review before commit. ## Commits ### PuPr new-items tail - **`feat(kernel): advanced HTTP proxy via WithKernelProxy`** (PECOBLR-3607) — fill the username / password / bypass_hosts args into the kernel's `set_proxy` setter (the kernel path passed the URL only). New `WithKernelProxy(url, user, pass, bypassHosts)` connector option; explicit proxy takes precedence over the env-var path. - **`test(kernel): cover Geography as the WKT sibling of Geometry`** (PECOBLR-3620) — verify-and-close: GEOGRAPHY maps to the same Arrow Utf8/WKT shape as GEOMETRY (no renderer needed). Parity + live-e2e (skip-on-reject) coverage. - **`feat(kernel): emit connection-config telemetry + fix connection-scoped error drop`** (PECOBLR-3627 / 3628) — emit `DriverConnectionParameters` at connect (kernel-path only; `mode=SEA`, `auth_mech`/`auth_flow` closed enums, proxy/arrow/query-tags/metric-view). Additive on top of #399's per-statement telemetry (not duplicated). The aggregator's `"error"` metric branch is routed correctly (a connection-scoped, no-statement error flushes standalone rather than being dropped), but it is **defensive**: no exported API emits a standalone `"error"` metric today, so the branch guards routing for a future producer rather than changing observable behavior now (clarified in review — see below). - **`docs(kernel): note token caching + client reuse are inherited from the kernel`** (PECOBLR-3606 / 3626) — verify-and-close, doc-only: both are handled inside the kernel below the C ABI. - **`feat(kernel): configurable HTTP retry / backoff`** (PECOBLR-3598) — wire `WithRetries` (RetryWaitMin/Max/RetryMax, incl. the disable form) into the kernel's new `set_retry_config` C-ABI setter, plus a kernel-only `WithKernelRetryOverallTimeout`. ### Datatype parity + decimal perf + RSS - **`feat(kernel): report column-type metadata on the kernel Rows path`** (PECOBLR-3692) — `kernelRows` implemented only `driver.Rows`, so `database/sql` fell back to `""` DatabaseTypeName / `interface{}` ScanType for every column vs Thrift's typed metadata (a silent regression for ORMs / typed-scan tooling that value-level tests can't see). New shared pure-Go `arrowscan.ColumnTypeInfoFor` maps Arrow → `{DatabaseTypeName, ScanType, Length}` matching Thrift for all 21 types; `kernelRows` now implements the 4 optional `RowsColumnType*` interfaces. Pure-Go guard + live `TestKernelThriftColumnTypeParity`. - **`perf(decimal): alloc-free exact decimal renderer (shared Thrift + kernel)`** (PECOBLR-3691) — rewrite `decimalfmt.ExactString` (on the shared render path) to allocate only the returned string: uint64 fast path renders into a stack scratch, `big.Int` only for true 128-bit magnitudes. Byte-identical output, pinned by an oracle-parity test (old impl kept verbatim as reference); 38.9ns/18B/1alloc → ~12ns/0B/0alloc. Live 50k-row multi-batch parity vs Thrift. - **`feat(kernel): tune CloudFetch in-memory chunks via WithKernelMaxChunksInMemory`** (PECOBLR-3691, RSS) — bound how many decompressed CloudFetch chunks the kernel holds in memory (default 16), the knob that trades throughput for peak RSS on large results (measured 265 MB @ chunks=4 vs 856 MB @ chunks=32 on a 3M-row query). Kernel-only option forwarded as the client-only `cloudfetch_max_chunks_in_memory` session conf, which the kernel folds into its result config and strips before the SEA wire — never reaching the server or the Thrift path. No new C-ABI symbol. ## Kernel dependency Two commits need new kernel behavior on the same kernel branch (PR #178): - `set_retry_config` C-ABI symbol (backoff commit), and - the `apply_client_result_overrides` reader that applies `cloudfetch_max_chunks_in_memory` (chunk-knob commit). `KERNEL_REV` is pinned to that branch's head (`ffa73c0`), so the tagged CI links both. **Re-pin to the squash-merge SHA once the kernel PR lands.** The other commits are driver-only and need no kernel change. ## Testing Both build paths green (`CGO_ENABLED=0 go test ./...` + `CGO_ENABLED=1 go test -tags databricks_kernel ./...`), `go vet` clean, gofmt clean, `golangci-lint run ./...` 0 issues. Pure-Go tests (option wiring, field-classification guards, ColumnType mapper, decimal oracle-parity, retry/proxy/telemetry resolvers, telemetry full-loop) run under `CGO_ENABLED=0`; tagged tests drive the real cgo setters; live e2e tests (proxy-rejects-bad-URL, proxy-credential-forwarding, geography, retry config/disable/overall-timeout, ColumnType parity, 50k-row decimal parity, CloudFetch chunk-knob drain) run against the staging warehouse. Each commit was reviewed with Isaac Review and its findings addressed before commit. ## Review findings addressed `fix(kernel): address code-review findings` (`29edbba`) resolves all 9 findings from the code review; a follow-up `isaac review` came back clean (0 posted comments). All threads are resolved. - **[Medium] `WithRetries(N, 0, 0)` silently reverted to the kernel default** — `kernelRetryConfig` now forwards the caller's `RetryMax` (with placeholder waits) on a degenerate range instead of returning `nil`. Because `WithDefaults()` runs before options, `WithRetries(n, 0, 0)` zeroed the waits and dropped the attempt count to the kernel's 5-retry default — a silent cross-backend divergence from Thrift. Only a zero-value range with no attempt count still falls back to the kernel default. New regression subtest. - **[Medium] Stale Thrift-path rejection message** — the message named only the two TLS options, but all five `WithKernel*` options trip the same gate; generalized to name the `WithKernel*` family, and the test now guards against a stale subset. - **[Medium] Connection-scoped error-drop branch targeted a dead path** — reframed the aggregator `"error"` branch and its test as **defensive** (no current producer), rather than an active behavior change. - **[Low] Uint types omitted from the coverage guard** — added `UINT8/16/32/64 → BIGINT/int64` to `ColumnTypeInfoFor` (matching the scanner's int64 widening) and to both guard tests. - **[Low] Doc comment named a nonexistent test** — replaced with the real guard names. - **[Low] `WithKernelMaxChunksInMemory` absent from the `doc.go` catalog** — added the bullet. - **[Low] No pure-Go coltype↔Thrift drift guard** — new `TestColumnTypeInfoMatchesThriftMapping` cross-checks `ColumnTypeInfoFor` against the Thrift `getScanType` / `GetDBTypeName` / length logic, so drift fails in `CGO_ENABLED=0` PR CI (not just the warehouse-gated nightly). - **[Low] Proxy credential/bypass forwarding untested** — new `TestKernelE2EProxyForwardsCredentials` captures the `Proxy-Authorization` a local CONNECT proxy sees and asserts it decodes to exactly `user:pass` (a slot swap would show `pass:user`). - **[Low] History-reference comments** — dropped the "(rejected before…)" / "Previously rejected —" clauses. --------- Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
added a commit
that referenced
this pull request
Jul 21, 2026
Bring PR #409 up to date after #412 (SEA PuPr tail: proxy / retry / max-chunks / telemetry) and #414 (ErrorCategory) landed on main. #409 and #412 both branched off #399 and extend the same kernel option surface, so the merge conflicted on 8 files; every conflict is additive-vs-additive and resolved by keeping both sides: - KERNEL_REV — re-pin to the #169 kernel tip 760625b, the only rev that carries BOTH #169's symbols (mTLS set_tls_client_certificate + abi_version, which #409's own code links) AND #178's set_retry_config (which the merged-in #412 code links). main's 9ac3f3d lacks the former; the old pin 12919b3 lacks the latter. 760625b = 12919b3's work + merged #178, same #169 lineage. - internal/config/config.go, internal/backend/kernel/config.go, connector.go, doc.go, kernel_config.go — keep both option families (mTLS client cert from #409; proxy / retry-overall-timeout / max-chunks from #412). The connector reject message and the ProxyURL doc take main's forward-looking wording. - kernel_test.go, kernel_experimental_test.go — de-interleave the spliced test additions (ABI-version test vs proxy/retry tests; disposition-map, option, and DeepCopy cases) into complete units, keeping every test from both. Verified on BOTH build paths: default pure-Go (go build/test ./...) and tagged (-tags databricks_kernel, linking the .a rebuilt at 760625b) — build, vet, and the touched-package tests all green. TestKernelExperimentalFieldsClassified passes, confirming no experimental field was dropped in the merge. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
added a commit
that referenced
this pull request
Jul 21, 2026
The kernel-backend CI job started failing after this branch re-pinned KERNEL_REV onto the #169 lineage that carries #178's configurable retry policy (5 attempts, 1s..60s backoff, 900s budget), which is now honored on the kernel path via WithRetries. The hermetic TLS tests (kernel_mtls_test.go) assert the TLS/mTLS handshake OUTCOME against a local httptest server. Their negative cases (handshake must fail: no client cert, untrusted CA, hostname mismatch) drive a connect that is meant to fail — and now retry it with escalating backoff. kernel_session_open is a blocking cgo call that cannot observe the Go ctx cancel mid-retry (a tracked deferral), so runKernelConnect's query goroutine outlived the test by ~34s per negative case and eventually wrote to the torn-down test pipe → "signal: broken pipe", failing the root package. Add WithRetries(-1, 0, 0) — the idiomatic disable form, honored on the kernel path — to runKernelConnect so a deliberately-failing connect settles after a single attempt. The whole file now runs in ~0.15s (was tens of seconds per negative case). No production code change; only the test's connect options. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updated m2m authenticator to use "all-apis" scope.
Added a new constructor function for m2m authenticator that allows client to pass in additional scopes.
Signed-off-by: Raymond Cypher raymond.cypher@databricks.com